-
Notifications
You must be signed in to change notification settings - Fork 264
P2 HLT timing: check on all expected json files to declare success #2662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
A new Pull Request was created by @mmusich for branch master. @akritkbehera, @cmsbuild, @iarspider, @raoatifshad, @smuzaffar can you please review it and eventually sign? Thanks. |
|
cms-bot internal usage |
|
test parameters:
|
|
@cmsbuild, please test |
|
-1 Failed Tests: HLTP2Timing Comparison SummarySummary:
|
522c9ac to
dafde32
Compare
|
Pull request #2662 was updated. |
|
@cmsbuild, please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8f44e5/50805/summary.html HLT P2 Timing: chart Comparison SummarySummary:
|
|
@smuzaffar can we merge this? it will help catching failures in tests that went unnoticed so far. |
| "$WORKSPACE/rundir/Phase2Timing_resources.json" | ||
| "$WORKSPACE/rundir/Phase2Timing_OnCPU_resources.json" | ||
| "$WORKSPACE/rundir/Phase2Timing_resources_NGT.json" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmusich , do old releases also generate these files? Note that we are running these tests for CMSSW_14_1_X and above.
For IBs tests, we are uploading Phase2Timing_resources*.json files , should we change it to upload mv Phase2Timing_*.json ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smuzaffar, thank you for reviewing.
do old releases also generate these files? Note that we are running these tests for CMSSW_14_1_X and above.
This is the current situation:
- CMSSW_16_1_X: generates all three files
- CMSSW_16_0_X: generates all three files
- CMSSW_15_1_X: generates
Phase2Timing_resources.jsonandPhase2Timing_resources_NGT.json - CMSSW_15_0_X (and below): generates
Phase2Timing_resources.json
how do you suggest to support a different number of files? I'd like to be fully explicit on the expected name of the json file: do we have means from the bash script to know the release version?
For IBs tests, we are uploading
Phase2Timing_resources*.jsonfiles , should we change it to upload mvPhase2Timing_*.json?
No. Actually thanks for pointing this out. This explains the mystery at cms-sw/cmssw#49279 (comment) !
I will rather change the name of the output json in order to match the expected pattern.
By the way can you show me which file is responsible for generating the IB tests?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at wha'ts done at cmssw-pr-test-config would:
CMSSW_VER=$CMSSW_VERSION
required_files=(
"$WORKSPACE/rundir/Phase2Timing_resources.json"
)
if [ "$CMSSW_VER" -ge 1501 ]; then
required_files+=(
"$WORKSPACE/rundir/Phase2Timing_resources_NGT.json"
)
fi
if [ "$CMSSW_VER" -ge 1600 ]; then
required_files+=(
"$WORKSPACE/rundir/Phase2Timing_OnCPU_resources.json"
)
fiwork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way can you show me which file is responsible for generating the IB tests?
@mmusich , that code is in jenkins job itself (https://cmssdt.cern.ch/jenkins/job/ib-run-hlt-p2-timing/configure)
echo "git cms-addpkg HLTrigger/Configuration" >> $WORKSPACE/run.sh
echo "cd $WORKSPACE/$RELEASE_FORMAT/src/HLTrigger/Configuration/python/HLT_75e33/test" >> $WORKSPACE/run.sh
echo "timeout $TIMEOUT ./runHLTTiming.sh > $WORKSPACE/upload/run.log 2>&1" >> $WORKSPACE/run.sh
echo "mv Phase2Timing_resources*.json $WORKSPACE/upload" >> $WORKSPACE/run.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at wha'ts done at cmssw-pr-test-config would:
some this should work, we can always run/request PR tests for old release cycles to check if it works properly or not. Also instead of converting CMSSW_VERSION (CMSSW_16_0_X) to CMSSW_VER (1600) in p2 timing script, I would suggest to update cms-bot/pr_testing/setup-pr-test-env.sh always have CMSSW_VERSION_NUMER and use it in p2 timign script (note that setup-pr-test-env.sh is sources in all PR test jobs to may be other test can also make use of it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to update
cms-bot/pr_testing/setup-pr-test-env.shalways have CMSSW_VERSION_NUMER and use it in p2 timign script (note that setup-pr-test-env.sh is sources in all PR test jobs to may be other test can also make use of it)
just to be sure I understand, the proposal is to add in cms-bot/pr_testing/setup-pr-test-env.sh this line:
CMSSW_VERSION_NUMBER=$CMSSW_VERSION`and then structure the if / else as
if [ "$CMSSW_VERSION_NUMBER" -ge 1501 ]; then
...?
If you confirm I'll provide a commit shortly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple CMSSW_VERSION_NUMBER=$CMSSW_VERSION is not enough. You need to add something like the following or feel free to implement it with simpler logic :-) All we need is to convert major and minor version of release cycle in. to two digits and concatenate then to get CMSSW_VERSION_NUMBER
CMSSW_VERSION_NUMBER=$CMSSW_VERSION
[ "${CMSSW_VERSION_NUMBER}" != "" ] || CMSSW_VERSION_NUMBER=${RELEASE_FORMAT}
CMSSW_MAJOR=0
CMSSW_MINOR=0
if [ "${CMSSW_VERSION_NUMBER}" != "" ] ; then
CMSSW_MAJOR=$(echo ${CMSSW_VER} | cut -d_ -f2)
CMSSW_MINOR=$(echo ${CMSSW_VER} | cut -d_ -f3)
fi
export CMSSW_VERSION_NUMBER=$(echo x0${CMSSW_MAJOR}x0${CMSSW_MINOR} | sed -r -e 's|x[0]*([0-9][0-9])|\1|g;s|^0||')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to implement it in 84487f1.
As in IBs tests uploading Phase2Timing_resources*.json, see cms-sw/cms-bot#2662 (comment)
- list of expected files is release dependent - changed setup-pr-test-env.sh to provide from the environment CMSSW version number
|
Pull request #2662 was updated. |
|
|
||
| if [ "$CMSSW_VERSION_NUMBER" -ge 1600 ]; then | ||
| required_files+=( | ||
| "$WORKSPACE/rundir/Phase2Timing_resources_OnCPU.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notice that for this work, it will require to have cms-sw/cmssw#49887 integrated (and backported).
|
test parameters:
|
As in IBs tests uploading Phase2Timing_resources*.json, see cms-sw/cms-bot#2662 (comment)
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8f44e5/50837/summary.html HLT P2 Timing: chart Comparison SummarySummary:
|
|
test parameters:
|
|
Pull request #2662 was updated. |
|
please test with cms-sw/cmssw#49887 |
|
please test for CMSSW_16_0_X |
|
please test for CMSSW_15_1_X |
I think for this we need to test with cms-sw/cmssw#49909 |
|
now |
Fixes #2658.
Title says it all, we need to check all (release-dependent) flavours after PR cms-sw/cmssw#49279